Skip to content

Fix impl-rlp for uint in primitive-types#117

Merged
ordian merged 3 commits into
paritytech:masterfrom
koushiro:fix-impl-rlp
Mar 27, 2019
Merged

Fix impl-rlp for uint in primitive-types#117
ordian merged 3 commits into
paritytech:masterfrom
koushiro:fix-impl-rlp

Conversation

@koushiro
Copy link
Copy Markdown
Contributor

@koushiro koushiro commented Mar 27, 2019

Signed-off-by: koushiro koushiro.cqx@gmail.com

It should be like

parity-common/rlp/src/impls.rs

Lines 259 to 263 in c64e1b5

impl_encodable_for_uint!(U256, 32);
impl_encodable_for_uint!(U128, 16);
impl_decodable_for_uint!(U256, 32);
impl_decodable_for_uint!(U128, 16);

@koushiro
Fix impl-rlp for uint in primitive-types
8ac568e 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@parity-cla-bot
Copy link
Copy Markdown

parity-cla-bot commented Mar 27, 2019

It looks like @koushiro signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@koushiro
Fix impl_uint_rlp
963724c 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@ordian
Copy link
Copy Markdown
Contributor

ordian commented Mar 27, 2019

Nice find, @koushiro!

There is also impl_uint_rlp!(U64, 1);.

I wonder whether we should fix impl_uint_rlp! macro itself, since it's confusing that it accepts size as bytes, while impl-serde and impl-codec as 8-byte words. cc @sorpaas (I believe it was first implemented in #86?) and cc @Robbepop. Also we have both ethereum feature in rlp and impl-rlp feature in primitive-types (probably for historical reasons, but maybe we can sort out what depends on what now).

@koushiro
Fix impl_uint_rlp
acee9b4 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Copy Markdown
Contributor Author

koushiro commented Mar 27, 2019

Nice find, @koushiro!

There is also impl_uint_rlp!(U64, 1);.

I wonder whether we should fix impl_uint_rlp! macro itself, since it's confusing that it accepts size as bytes, while impl-serde and impl-codec as 8-byte words. cc @sorpaas (I believe it was first implemented in #86?) and cc @Robbepop. Also we have both ethereum feature in rlp and impl-rlp feature in primitive-types (probably for historical reasons, but maybe we can sort out what depends on what now).

It seems that there is no U64 type in primitive-types, and I have fixed the impl_uint_rlp! macro. PTAL, @ordian .

ordian
ordian approved these changes Mar 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would be nice to have tests to catch that kind of bugs.

CI failure is unrelated (travis on Windows is broken for ages, I'll open a separate PR to migrate back to appveyor).

@koushiro
Copy link
Copy Markdown
Contributor Author

koushiro commented Mar 27, 2019

LGTM, but would be nice to have tests to catch that kind of bugs.

CI failure is unrelated (travis on Windows is broken for ages, I'll open a separate PR to migrate back to appveyor).

Maybe we could add some tests from https://github.com/chainx-org/chainx-common/blob/develop/primitive-types/src/tests.rs

@ordian
Copy link
Copy Markdown
Contributor

ordian commented Mar 27, 2019

Ok, let's merge this PR. A PR with tests is very welcome.

@ordian ordian merged commit 0ddf13c into paritytech:master Mar 27, 2019
ordian added a commit that referenced this pull request Mar 27, 2019
@ordian
Merge branch 'master' into ao-abstract-kvdb-with-cache
4ce9089 
* master:
  Fix impl-rlp for uint in primitive-types (#117)
@koushiro koushiro deleted the fix-impl-rlp branch May 27, 2019 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ordian ordian ordian approved these changes

@sorpaas sorpaas sorpaas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@koushiro @parity-cla-bot @ordian @sorpaas